Fix computed values to be computed after all set_options #690

ghost · 2021-08-15T12:58:26Z

What this pr does

Make "true-color = ***" from gitconfig works

How it was

It was not working from original source.

So I make it works.

This still give prior to the config from cli. Also 24-bit-color can still work.

Motivaiton

I'm trying to integrate delta inside tig.

#689

However, if you allow user to use RGB, it's almost impossible inside tig since it uses ncurses. So I'm trying to limit only 8bit colors. Then I found this bug.

The new test fails before 7ed3c2c

dandavison · 2021-08-16T00:22:11Z

Hi again @ulwlu! Great, thanks for this. I've pushed one commit to your branch adding a test that is intended to reproduce the bug that you are fixing. But there's one thing I'm confused about:

My test fails on master and passes on your branch. However my test tests true-color = never whereas the new code you've added is inside a if opt.true_color == "auto" statement. I haven't had time yet to figure out why my test passes on your branch. Would you be able to look at the test, and check everything makes sense, and add any further test cases you think are needed?

ghost · 2021-08-16T03:35:07Z

Hi @dandavison thank you for your review.

This is bacause cli.rs set opt.true-color to 'auto' if '--true-color' option is not given. That is the default value given by struct-opt.

I think it's little bit confusing, maybe we can remove the default value and change match condition inside set_true_color function.

dandavison · 2021-08-16T17:18:58Z

src/options/set.rs

+                arg_matches,
+                option_names,
+                false
+            );


true_color is present in the main call to set_options! on l.182. I think we need to know why that call is not working -- it feels wrong to make two set_options! calls for true_color.

Oh I see, and sorry. I missed that and found opt.true_color is set correctly (not exact correct because it's not computed into bool) from gitconfig after set_options. I'm maybe going to fix.

dandavison · 2021-08-16T18:14:01Z

This is bacause cli.rs set opt.true-color to 'auto' if '--true-color' option is not given. That is the default value given by struct-opt.

Thanks for explaining that!

ghost · 2021-08-17T07:45:37Z

Thanks to your pointing out, I found some configurations which is to be computed depends on opt.*** value must be after set_options which gets value from gitconfig. It is because gitconfig value will never be computed.

I'm going to think what way is good.

ghost · 2021-08-17T09:19:38Z

@dandavison

Hi I fixed and change this PR's purpose.

Change ComputedValues's computing place after all set_options
- It is because some of them depends on opt.** value but they have chance to miss gitconfig values
The ComputedValues which have a default value and computed before set_options were going wrong.
- The ComputedValues which have a default value: paging-mode, inspect-raw-lines, and true-color
- The ComputedValues which computed before set_options: true-color
- So, only true-color was going wrong

I move set_width after set_options as well, so this func now doesn't need to set_options inside it.

I added a test for these computed values are setting correctly into config.
I removed an unused ComputedValue.

dandavison · 2021-08-20T16:52:41Z

Awesome, thanks very much for these fixes @ulwlu. I did some manual testing to check that width worked when set as a command-line option, and in gitconfig, and via git -c delta.width=xxx ....

ghost · 2021-08-20T16:54:39Z

@dandavison thanks!

Btw, integrating tig with delta is almost finished. Now I'm fixing little bugs.

jonas/tig#1140

dandavison · 2021-08-20T17:05:15Z

Btw, integrating with delta is almost finished. Now I'm fixing little bugs.

Awesome. Those screenshots look really nice. (I just tried compiling but got an error

src/line.c:245:3: error: implicit declaration of function 'init_extended_pair' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
                init_extended_pair(++cnt, fg, bg);

)

ghost · 2021-08-20T17:09:49Z

Umm, init_expanded_pair is only available with ncurse upper than version 6.1, so compiling option may get wrong (such as linking with different path).

I also strrugled at that building. If this get merged jonas will release the new binary anyway, so maybe trying after that is easier...

dandavison · 2021-08-20T17:25:47Z

OK thanks (my Mac has 5.4, but I saw your instructions for building and linking against ncurses).

* upstream/master: Refactor: use Option to model sometimes-null highlighter (dandavison#698) Add some test coverage for truncate_str with a multibyte unicode character Use "syntax theme" terminology in show-syntax-themes output (dandavison#697) Fix deadlock in `git diff` mode (dandavison#695) Fix empty line highlighting (dandavison#642) Revert "Add failing test that gitconfig insteadOf is honored" Add failing test that gitconfig insteadOf is honored Support `insteadOf` replacements in git remote URLs Hold GitConfig in main Config struct Revert "Support `insteadOf` replacements in git remote URLs" Support `insteadOf` replacements in git remote URLs Refactoring for dandavison#693 (dandavison#696) Make it possible to jump between files when navigate is active (dandavison#684) Bump dev version number Compile delta from source in dockerfile Fix computed values to be computed after all set_options (dandavison#690) Remove unnecessary borrows (dandavison#692) Bump bitflags from 1.3.1 to 1.3.2 (dandavison#691) Bump git2 from 0.13.20 to 0.13.21 (dandavison#687)

ulwlu and others added 2 commits August 15, 2021 21:54

Make truecolor option honorable from gitconfig

7ed3c2c

Add test that true-color can be read from git config

93e0819

The new test fails before 7ed3c2c

dandavison reviewed Aug 16, 2021

View reviewed changes

ulwlu added 3 commits August 17, 2021 18:10

Place computing process after set_options

62495a2

Add test for checking if values requre computing set correctly

1ee71c7

Remove unnecessary computed values candidate

d7ac0ce

ghost changed the title ~~Make true-color option honorable from gitconfig~~ Fix computed values to be computed after all set_options Aug 17, 2021

dandavison merged commit 9fa449e into dandavison:master Aug 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix computed values to be computed after all set_options #690

Fix computed values to be computed after all set_options #690

ghost commented Aug 15, 2021

dandavison commented Aug 16, 2021

ghost commented Aug 16, 2021

dandavison Aug 16, 2021

ghost Aug 17, 2021 •

edited by ghost

Loading

dandavison commented Aug 16, 2021

ghost commented Aug 17, 2021

ghost commented Aug 17, 2021

dandavison commented Aug 20, 2021

ghost commented Aug 20, 2021 •

edited by ghost

Loading

dandavison commented Aug 20, 2021

ghost commented Aug 20, 2021

dandavison commented Aug 20, 2021

Fix computed values to be computed after all set_options #690

Fix computed values to be computed after all set_options #690

Conversation

ghost commented Aug 15, 2021

What this pr does

How it was

Motivaiton

dandavison commented Aug 16, 2021

ghost commented Aug 16, 2021

dandavison Aug 16, 2021

Choose a reason for hiding this comment

ghost Aug 17, 2021 • edited by ghost Loading

Choose a reason for hiding this comment

dandavison commented Aug 16, 2021

ghost commented Aug 17, 2021

ghost commented Aug 17, 2021

dandavison commented Aug 20, 2021

ghost commented Aug 20, 2021 • edited by ghost Loading

dandavison commented Aug 20, 2021

ghost commented Aug 20, 2021

dandavison commented Aug 20, 2021

ghost Aug 17, 2021 •

edited by ghost

Loading

ghost commented Aug 20, 2021 •

edited by ghost

Loading